Skip to content

Remove "-Werror" from default CMake builds #255

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

skaravos
Copy link
Contributor

@skaravos skaravos commented Nov 5, 2024

  • Provides a workaround for #249

  • Removes -Werror from the default set of compiler warnings for GCC to lessen the impact on downstream package maintenance.

  • Adds two new cmake options:

    1. LOGURU_ENABLE_COMPILER_WARNINGS

      • enabled by default when the project is compiled as the top-level cmake project,
        can be explicitly set to FALSE to opt-out of compiler warnings
    2. LOGURU_WARNINGS_AS_ERRORS

      • disabled by default, can be explicitly set to TRUE to opt-in to treating warnings as errors

Changes:
- Provides a workaround for
  [emilk#249](emilk#249)
- Adds an option that can be used to disable compile warnings for
  the loguru library. This could be useful for users who just want to
  build the library and dont care about the warnings.
  (or they just aren't in a position to fix them anyways).

Notes:
- The default behaviour remains the same: compiler-warnings are enabled
  by default when the project is built as a top-level project, and
  disabled by default when the project is included as a sub-project with
  either add_subdirectory() or FetchContent()
Notes:
- This is needed to quiet a cmake warning about non-cache variables
  being used to set loguru cmake `options()`, because loguru's minimum
  cmake version is still 3.10 and this wasn't allowed until cmake 3.13.

  ```
  # CMakeLists.txt

  # prior to cmake 3.13 the user would need to set options as cache vars
  set(LOGURU_ENABLE_COMPILER_WARNINGS FALSE CACHE BOOL "" FORCE)

  # cmake 3.13 and onwards the sub-project options respect normal vars.
  set(LOGURU_ENABLE_COMPILER_WARNINGS FALSE)

  add_subdirectory(loguru)
  ```
Changes:
- Provides a workaround for [emilk#249](emilk#249)
- Removed `-Werror` from the default compiler warnings for GCC.
- Added an option that can be used to opt-in to warnings-as-errors for
  all three supported compilers (gcc, clang, msvc) if desired.
@drupol
Copy link

drupol commented Nov 6, 2024

Just for my own information, what's the technical difference between this method and fixing the issue in the CPP code directly? Like these 2 patches are doing (https://github.com/virtuosonic/loguru/commit/e1ffdc4149083cc221d44b666a0f7e3ec4a87259.patch and https://github.com/virtuosonic/loguru/commit/743777bea361642349d4673e6a0a55912849c14f.patch) ?

@skaravos
Copy link
Contributor Author

skaravos commented Nov 7, 2024

Just for my own information, what's the technical difference between this method and fixing the issue in the CPP code directly? Like these 2 patches are doing (https://github.com/virtuosonic/loguru/commit/e1ffdc4149083cc221d44b666a0f7e3ec4a87259.patch and https://github.com/virtuosonic/loguru/commit/743777bea361642349d4673e6a0a55912849c14f.patch) ?

This PR addresses a different concern entirely.
You're right in pointing out that the proper fix to #249 is applying those two patches to the C++ code, and it would certainly be worth having a PR's submitted for those.

However, even if the C++ code gets fixed it doesn't address the fact that the default out-of-the-box CMake build for the project should not have -Werror enabled in the first place.
I didn't really even mean to have -Werror added to the CMake file (as evidenced by the fact I only had the flag enabled for GCC, but not clang or MSVC) it was an oversight and this PR addresses that.

Notes:
- Realized I must've dropped the actual most important commit while
  rebasing this branch before submission, sheesh
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants